Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BlockRng: make core public and remove inner() and inner_mut() #425

Merged
merged 1 commit into from
May 7, 2018

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented May 3, 2018

@pitdicker what do you think?

I believe I found a bug regarding half_used...

...which brings me to a related point: didn't you mention trying to transmute the output of generate from [u64] to [u32] then using the same BlockRng code in (or instead of) BlockRng64?

I don't like the way half_used works right now; it could make it difficult to reproduce RNG output in other implementations.

@dhardy dhardy requested a review from pitdicker May 3, 2018 15:39
@dhardy dhardy added E-question Participation: opinions wanted B-API Breakage: API P-high Priority: high labels May 7, 2018
Copy link
Contributor

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I looked over your PR but forgot to reply. Looks good to me. Feel free to merge if you really think it is better 👍.

@pitdicker
Copy link
Contributor

...which brings me to a related point: didn't you mention trying to transmute the output of generate from [u64] to [u32] then using the same BlockRng code in (or instead of) BlockRng64?

Yes, but I forgot that it doesn't work on big-endian #391.

@dhardy dhardy merged commit 2c3d34d into master May 7, 2018
@dhardy dhardy deleted the blockrng branch May 7, 2018 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-API Breakage: API E-question Participation: opinions wanted P-high Priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants